Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

expression: bug fix for vectorized casting real as time #19485

Merged
merged 7 commits into from
Aug 29, 2020
Merged

expression: bug fix for vectorized casting real as time #19485

merged 7 commits into from
Aug 29, 2020

Conversation

dyzsr
Copy link
Contributor

@dyzsr dyzsr commented Aug 26, 2020 •
edited
Loading

What problem does this PR solve?

Issue Number: close #17809

Problem Summary:

Casting from column value 0.0 of real type to datetime type produces wrong result NULL.

CREATE TABLE t0(c0 DOUBLE);
INSERT INTO t0(c0) VALUES (0);
SELECT CAST(t0.c0 AS DATETIME) FROM t0; -- expected: {0000-00-00 00:00:00}, actual: {NULL}

But casting from constant produces the expected result 0000-00-00 00:00:00.

SELECT CAST(0.0 AS DATETIME) FROM t0; -- {0000-00-00 00:00:00}

What is changed and how it works?

What's Changed:

  • builtinCasrRealAsTimeSig.vecEvalTime will treat real value 0.0 as a special case, and assign ZeroTime to the result.

How it Works:

  • the non-vectorized evalTime has this special case but vecEvalTime does not.
  • add this special case to vecEvalTime

Related changes

Check List

Tests

  • Unit test

Release note

  • fix incorrect result of vectorized casting real as time

@dyzsr dyzsr requested a review from a team as a code owner August 26, 2020 10:55
@dyzsr dyzsr requested review from SunRunAway and removed request for a team August 26, 2020 10:55
zz-jason
zz-jason reviewed Aug 26, 2020
View reviewed changes
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a unit test

lzmhhh123
lzmhhh123 reviewed Aug 27, 2020
View reviewed changes
Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 27, 2020
zz-jason
zz-jason approved these changes Aug 27, 2020
View reviewed changes
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Aug 27, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 27, 2020
@zz-jason
Copy link
Member

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 27, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@dyzsr merge failed.

@zz-jason
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 19527
  • 19528

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@dyzsr merge failed.

@zz-jason
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 19528

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@dyzsr merge failed.

@zz-jason
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@dyzsr merge failed.

@zz-jason
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@dyzsr merge failed.

@ngaut ngaut merged commit 844c114 into pingcap:master Aug 29, 2020
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Aug 29, 2020
@dyzsr @ti-srebot
cherry pick pingcap#19485 to release-3.0
1b929a1 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Aug 29, 2020
Closed
@ti-srebot
Copy link
Contributor

cherry pick to release-3.0 in PR #19593

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Aug 29, 2020
@dyzsr @ti-srebot
cherry pick pingcap#19485 to release-4.0
5b9673d 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Aug 29, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #19594

This was referenced Aug 31, 2020
Closed
Merged
Merged
zz-jason pushed a commit that referenced this pull request Sep 7, 2020
@ti-srebot
expression: bug fix for vectorized casting real as ... (#19485) (#19594)
44c518c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzmhhh123 lzmhhh123 lzmhhh123 left review comments

@zz-jason zz-jason zz-jason approved these changes

@ti-srebot ti-srebot ti-srebot approved these changes

@SunRunAway SunRunAway Awaiting requested review from SunRunAway

Assignees
No one assigned
Labels
component/expression status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bug-fix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

P1-[4.0 bug hunting]-[SQL Plan Management]-Incorrect result for CAST to DATETIME
6 participants
@dyzsr @zz-jason @ti-srebot @lzmhhh123 @ngaut @SunRunAway